Skip to content

BUG: Fix delegation behaviour with atleast_3d #514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

BUG: Fix delegation behaviour with atleast_3d #514

wants to merge 1 commit into from
+109 −20

Conversation

@Enderdead
Copy link
Contributor

@Enderdead Enderdead commented Nov 6, 2025

This PR fixes the bad behavior reported in scipy/scipy#23929 when ndim=3.

It removes delegation when ndim > 2 and adds additional tests.
These tests reproduce the issue while avoiding input arrays of size 1.

I'm not entirely happy with these new tests, but they provide a reasonable starting point for further iteration.

Feedback is welcome.

@lucascolley lucascolley self-requested a review November 6, 2025 21:50
@lucascolley lucascolley added the bug Something isn't working label Nov 6, 2025
@lucascolley lucascolley added this to the 0.9.1 milestone Nov 7, 2025
lucascolley
lucascolley reviewed Nov 7, 2025
View reviewed changes
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I wonder whether, rather than checking for exact equality in all of these tests, we should just check for expected shapes? Probably good to still keep one sanity-check test that we aren't unexpectedly modifying the values, but otherwise the tests are quite verbose.

Thoughts @Enderdead ?

@Enderdead
Copy link
Contributor Author

Enderdead commented Nov 9, 2025

@lucascolley You're right — it will be easier to understand and maintain. I'll propose another version ASAP.

@Enderdead
Copy link
Contributor Author

Enderdead commented Nov 9, 2025

@lucascolley I’m proposing this new version with a hash method to perform a semi–sanity check.
Sorry if it’s still a bit heavy — it’s difficult for me to gauge how far we can go with simplifying this test.

@lucascolley lucascolley self-requested a review November 9, 2025 17:37
@lucascolley lucascolley changed the title BUG: Fix delegation behaivour with atleast_3d. BUG: Fix delegation behaivour with atleast_3d Nov 13, 2025
lucascolley
lucascolley approved these changes Nov 13, 2025
View reviewed changes
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Enderdead, I like the approach!

@Enderdead Enderdead changed the title BUG: Fix delegation behaivour with atleast_3d BUG: Fix delegation behaviour with atleast_3d Nov 13, 2025
@Enderdead
Copy link
Contributor Author

Enderdead commented Nov 13, 2025 •
edited
Loading

I replaced the asarray + range combination with xp.arange. I also squashed the three commits.

Do you want me to add @pytest.mark.xfail_xp_backend(Backend.SPARSE, reason="no arange", strict=False) in order to pass the new tests ?

@lucascolley
Copy link
Member

lucascolley commented Nov 13, 2025 •
edited
Loading

since sparse has no arange, I think we can generate arrays with np.arange and wrap with xp.asarray.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lucascolley lucascolley lucascolley approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

0.9.1

Development

Successfully merging this pull request may close these issues.

2 participants

@Enderdead @lucascolley